Minor comment: I think that a better name for this method would be simply duration.

The reason for this is due to the implication behind the phrasing "calculate duration". In that case we're explicitly saying that the duration will be calculated by this method (using the equation on line 30). This is in contrast to a method that simply returns the value of a previously saved/assigned instance variable.

Naming a method in a way that indicates how the method works is considered to be a more fragile approach than avoiding those details. Because if we end up needing to change the method's code, the name may no longer be accurate.

For this reason the Ruby convention for method names like this is to stick with a noun rather than a verb+object combination phrase. Examples:

This line is not doing what you probably intended it to do.

Please review this code and figure out what should be changed. Once you know what should be changed -- stop! Before you actually make the change, try to write a test first!

Figure out what kind of test would be necessary to verify that line 28 is working, then write that test. Running your tests should then show that the new one is failing. After that you can implement your fix, and run the tests again to verify that it worked.

If you have any trouble following the above suggestions, let me know!

Less important comment on this unless statement: This code is probably a bit "too fragile", meaning it's coded with a very strict expectation of what data values might be in the @status variable.

As written, we're expecting that the only valid values for @status are :AVAILABLE and :UNVAILABLE. This is fine for now as the project requirements only involve those two options, but we could definitely imagine additional status options (like :LOCKED for a driver who has been banned or otherwise prevented from using the system).

In that case it might be simpler to write the conditional this way:

if @status.nil?
   # set @status to :UNAVAILABLE
end

With that above code we have logic that makes :UNAVAILABLE the default status, but we aren't preventing other values from being provided.

This check should happen as the first thing within this method.

Traditionally, raising an error like we're doing on this line means that the application stops right then. However, it is possible to use rescue blocks to catch those errors and allow the program to continue running.

In that situation, we will have added an invalid trip to the @driven_trips array, and the rest of the program may function incorrectly and result in more bugs or more incorrect results for users.

Finally, as with my previous comment, I suggest fixing this method by moving the check and raise lines to the beginning of the method. However, first y'all should write a test will fail until the method is fixed.

Minor comment: This check could be implemented as an "early exit" as well:

def average_rating
  return 0 if @driven_trips.empty?

  # ... rest of the method

I also think it's worth considering whether zero is the correct return value when a driver has not yet driven any trips.

I don't think this check is necessary for the total_revenue. We have the same check in average_rating, but it's necessary there to avoid potentially dividing by zero on line 56.

In this case we have no division, so it should be fine to run all of the code when @driven_trips is empty. However, this check does correct for one bug (but only in one case): without this check the method would return a negative value for drivers without any trips.

Clearly that would be an invalid result, so it's good that we're avoiding it. However, we're still possibly running into that bug when @driven_trips is not empty. If there was a driver who had one trip but the cost for that trip was less than $1.65, then total_revenue will still return a negative value.

So, my suggestion would be to remove the check about @driven_trips being empty, and instead use the same calculation in all cases. Then, have the method return a minimum value of zero: return [0, total].max.

Very minor: This test could be further dried up:

it "is set up for specific attributes and data types" do
  {id: Integer, name: String, vin: String, status: Symbol, driven_trips: Array}.each do |prop, type|
    expect(@driver).must_respond_to prop
    expect(@driver.send(prop)).must_be_kind_of type
  end
end

This line would be a good candidate for being moved into its own "helper" method. Doing so would be in keeping with the "separation of concerns" concept discussed in POODR.

I really like this method! When we have constraints/requirements that are somewhat "implicit", factory methods like this can help ensure that we don't write code that accidentally breaks those constraints.

In this case the Trip class has an implicit constraint that the associated Passenger and Driver must also include the trip instance in their own arrays. Because we have this factory method then we can more easily enforce that the only way to create a new Trip instance in this project is to call the create_trip method.

Minor note: Because the list of trips is provided as a parameter rather than assumed to be the @trips instance variable, this method could actually be a class method. That is usually the convention for factory methods, actually.

I would also consider that providing the trip list, and having code to push the new trip into that list, is probably a violation of "separation of concerns". It's possible that we could want to create a trip and not put it into an array, which is not supported by the current version of this method.

Minor comment: I think the correct behavior here would be for TripDispatcher#find_driver to return nil when no driver was found with the given ID.

The reasoning for this is that technically it's not an invalid argument -- the ID was actually an integer, it just didn't happen to match anything in our data set. Another reason for preferring to return nil is because Rails (which the all and find methods in these projects are based upon) also works that way.

This is a solid test! We could expand on it by requesting a second trip after the first one, and then verifying that we got the next available driver (and not driver #5 a second time).